Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix -Z print-type-sizes for generators with discriminant field ordered first #105623

Merged
merged 2 commits into from
Dec 15, 2022
Merged

Fix -Z print-type-sizes for generators with discriminant field ordered first #105623

merged 2 commits into from
Dec 15, 2022

Conversation

compiler-errors
Copy link
Member

Fixes #105589
Fixes #105591

@rustbot
Copy link
Collaborator

rustbot commented Dec 12, 2022

r? @Nilstrieb

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 12, 2022
@compiler-errors
Copy link
Member Author

compiler-errors commented Dec 12, 2022 •
edited
Loading

Confirmed manually that #105589 is fixed by this by running RUSTFLAGS=-Zprint-type-sizes cargo +rust build --release on cargo-binstall.

@compiler-errors compiler-errors force-pushed the generator-type-size-fix branch 2 times, most recently from e6e0368 to c85ce4c Compare December 12, 2022 19:23
Noratrieb
Noratrieb requested changes Dec 13, 2022
View reviewed changes
compiler/rustc_ty_utils/src/layout.rs
_ => Size::ZERO,
};

if layout.fields.offset(tag_field) >= variant_size {
Copy link
Member

@Noratrieb Noratrieb Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while to understand why this change was correct, probably mostly because the variant_size variable is a little confusingly named. Maybe something like last_field_end would better?

Thinking about this a little more, what this code really wants to do is variant_size.max(tag_end), maybe that would also make it simpler. Actually no, that would be incorrect if the discriminant was at the end.

Copy link
Member Author

@compiler-errors compiler-errors Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is any clearer with the name last_field_end, because it really is the variant size (plus the upvars), modulo a hack to deal with needing to conditionally subtract one from the variant size depending on where the layout code chooses to put the tag.

Anyways, what I can do is leave a more detailed comment explaining why this is needed. This is probably still broken, but I don't know if it's worth taking much more time to fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true, all of this code is trying to fit generator layout into a normal layout shaped hole and it's not going great. The current name is fine too and yeah, spending too much time on this is probably not worth it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But a quick comment would be nice.

Noratrieb
Noratrieb approved these changes Dec 14, 2022
View reviewed changes
Copy link
Member

@Noratrieb Noratrieb left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the comment, r=me once CI is green.

@compiler-errors
Copy link
Member Author

@bors r=Nilstrieb

@bors
Copy link
Contributor

bors commented Dec 14, 2022

📌 Commit bdc3c4b has been approved by Nilstrieb

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 14, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 14, 2022
@matthiaskrgr
Rollup merge of rust-lang#105623 - compiler-errors:generator-type-siz…
fbe00f0 
…e-fix, r=Nilstrieb

Fix `-Z print-type-sizes` for generators with discriminant field ordered first

Fixes rust-lang#105589
Fixes rust-lang#105591
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 14, 2022
@matthiaskrgr
Rollup merge of rust-lang#105623 - compiler-errors:generator-type-siz…
d9e36a4 
…e-fix, r=Nilstrieb

Fix `-Z print-type-sizes` for generators with discriminant field ordered first

Fixes rust-lang#105589
Fixes rust-lang#105591
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 14, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 15, 2022
@matthiaskrgr
Rollup merge of rust-lang#105623 - compiler-errors:generator-type-siz…
601eedd 
…e-fix, r=Nilstrieb

Fix `-Z print-type-sizes` for generators with discriminant field ordered first

Fixes rust-lang#105589
Fixes rust-lang#105591
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 15, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 15, 2022
@matthiaskrgr
Rollup merge of rust-lang#105623 - compiler-errors:generator-type-siz…
6cb3c62 
…e-fix, r=Nilstrieb

Fix `-Z print-type-sizes` for generators with discriminant field ordered first

Fixes rust-lang#105589
Fixes rust-lang#105591
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 15, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 15, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 15, 2022
@bors
Auto merge of rust-lang#105746 - matthiaskrgr:rollup-sz3grbv, r=matth…
984eab5 
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#104592 (Ensure async trait impls are async (or otherwise return an opaque type))
 - rust-lang#105623 (Fix `-Z print-type-sizes` for generators with discriminant field ordered first)
 - rust-lang#105627 (Auto traits in `dyn Trait + Auto` are suggestable)
 - rust-lang#105633 (Make `report_projection_error` more `Term` agnostic)
 - rust-lang#105683 (Various cleanups to dest prop)
 - rust-lang#105692 (Add regression test for rust-lang#104678)
 - rust-lang#105707 (rustdoc: remove unnecessary CSS `kbd { cursor: default }`)
 - rust-lang#105715 (Do not mention long types in E0599 label)
 - rust-lang#105722 (more clippy::complexity fixes)
 - rust-lang#105724 (rustdoc: remove no-op CSS `.scrape-example .src-line-numbers { margin: 0 }`)
 - rust-lang#105730 (rustdoc: remove no-op CSS `.item-info:before { color }`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5d24760 into rust-lang:master Dec 15, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 15, 2022
@compiler-errors compiler-errors deleted the generator-type-size-fix branch August 11, 2023 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Noratrieb Noratrieb Noratrieb approved these changes

Assignees

@Noratrieb Noratrieb

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.68.0
4 participants
@compiler-errors @rustbot @bors @Noratrieb